chore(lib): remove error branch from LUT if it is unreachable #386
Conversation
|
Hello @RustyYato! Thank you for your PR! Can you implement a small test that generates the LUT and checks that, indeed, the error branch is removed if it is unreachable? Tell me if you need help with setting up the test :-) |
|
Would you like a test like in |
|
friendly ping @jeertmans 😄 |
|
Ooos sorry @RustyYato! yes the easiest way to test this might be to test against the output of the logos-cli. So you can generate a very basic scenario: one lexer that has a useful error branch, and one whose error branch was removed :) To check that your tests work, you can remove your patch and first check that they fail, then apply your patch and check they now work :) |
|
Hey, I will be out for the next few days, but I'll get back to this once I'm back. 😀 |
|
Hmm, it looks like the lookup table isn't taken in some cases (specifically the one in the related issue). So there may be some other low hanging fruit to remove error cases. But I'll add a test for one that does use the LUT and solve the other cases separately. |
|
I've just updated the tests in |
|
Looks great, thanks @RustyYato! I have 2 remarks / suggestions:
Don't hesitate to give your opinion on this :-) |
This reverts commit 0dfcf00.
|
Ok, in that case I think it would be nice to make it easy to add new tests and update old tests if codegen changes. I can rebase the changes right before merging to clean up the history if you would like. I find rebases during reviews make reviewing harder so didn't do that yet. |
|
friendly ping @jeertmans 🙂 |
jeertmans
changed the title
|
Hey @RustyYato, I am very sorry about my delay, was very busy working on my own projects and work, so I put those reviews on hold. Anyway, I think your work is great and deserves to be merged! Thanks for your great work on this! At first, I was waiting because I wanted to think a bit about the best (and cleanest) way to handle those kind tests, but let's merge this and see if we need to improve it (or not) in the future! |
|
Not a problem, thanks for the reviews! |
[](https://renovatebot.com) This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [logos](https://logos.maciej.codes/) ([source](https://togithub.com/maciejhirsz/logos)) | dependencies | patch | `0.14.0` -> `0.14.1` | --- ### Release Notes <details> <summary>maciejhirsz/logos (logos)</summary> ### [`v0.14.1`](https://togithub.com/maciejhirsz/logos/releases/tag/v0.14.1): 0.14.1 - Debug feature and fixes #### What's Changed - fix(doc): reset logos2 to logos by [@​jeertmans](https://togithub.com/jeertmans) in [https://github.com/maciejhirsz/logos/pull/372](https://togithub.com/maciejhirsz/logos/pull/372) - chore(book): add JSON-borrowed parser example by [@​jeertmans](https://togithub.com/jeertmans) in [https://github.com/maciejhirsz/logos/pull/373](https://togithub.com/maciejhirsz/logos/pull/373) - Add Rc<T> and Arc<T> sources by [@​InfiniteCoder01](https://togithub.com/InfiniteCoder01) in [https://github.com/maciejhirsz/logos/pull/340](https://togithub.com/maciejhirsz/logos/pull/340) - Fix unicode dot by [@​RustyYato](https://togithub.com/RustyYato) in [https://github.com/maciejhirsz/logos/pull/376](https://togithub.com/maciejhirsz/logos/pull/376) - chore(docs): cleanup examples by [@​jeertmans](https://togithub.com/jeertmans) in [https://github.com/maciejhirsz/logos/pull/381](https://togithub.com/maciejhirsz/logos/pull/381) - chore(lib): add debug feature by [@​jeertmans](https://togithub.com/jeertmans) in [https://github.com/maciejhirsz/logos/pull/382](https://togithub.com/maciejhirsz/logos/pull/382) - Cleanup unused Source features by [@​kmicklas](https://togithub.com/kmicklas) in [https://github.com/maciejhirsz/logos/pull/335](https://togithub.com/maciejhirsz/logos/pull/335) - chore(deps): bump peaceiris/actions-mdbook from 1 to 2 by [@​dependabot](https://togithub.com/dependabot) in [https://github.com/maciejhirsz/logos/pull/387](https://togithub.com/maciejhirsz/logos/pull/387) - Fix `Lexer::clone` leak and UB + tests by [@​Jakobeha](https://togithub.com/Jakobeha) in [https://github.com/maciejhirsz/logos/pull/390](https://togithub.com/maciejhirsz/logos/pull/390) - fix(lib): correctly handle miss for loop in loop by [@​lukas-code](https://togithub.com/lukas-code) in [https://github.com/maciejhirsz/logos/pull/393](https://togithub.com/maciejhirsz/logos/pull/393) - chore(lib): remove error branch from LUT if it is unreachable by [@​RustyYato](https://togithub.com/RustyYato) in [https://github.com/maciejhirsz/logos/pull/386](https://togithub.com/maciejhirsz/logos/pull/386) - fix(docs): typo by [@​joerivanruth](https://togithub.com/joerivanruth) in [https://github.com/maciejhirsz/logos/pull/396](https://togithub.com/maciejhirsz/logos/pull/396) - chore(docs): Adds graph debug documentation to book by [@​afreeland](https://togithub.com/afreeland) in [https://github.com/maciejhirsz/logos/pull/379](https://togithub.com/maciejhirsz/logos/pull/379) - chore: drop python linting frmo pre-commit-config by [@​LeoDog896](https://togithub.com/LeoDog896) in [https://github.com/maciejhirsz/logos/pull/403](https://togithub.com/maciejhirsz/logos/pull/403) - refactor: don't use deprecated max_value() method by [@​LeoDog896](https://togithub.com/LeoDog896) in [https://github.com/maciejhirsz/logos/pull/404](https://togithub.com/maciejhirsz/logos/pull/404) - chore(version): bump logos version to 0.14.1 by [@​jeertmans](https://togithub.com/jeertmans) in [https://github.com/maciejhirsz/logos/pull/409](https://togithub.com/maciejhirsz/logos/pull/409) - fix(docs): change old 0.14.0 by [@​jeertmans](https://togithub.com/jeertmans) in [https://github.com/maciejhirsz/logos/pull/410](https://togithub.com/maciejhirsz/logos/pull/410) #### New Contributors - [@​InfiniteCoder01](https://togithub.com/InfiniteCoder01) made their first contribution in [https://github.com/maciejhirsz/logos/pull/340](https://togithub.com/maciejhirsz/logos/pull/340) - [@​RustyYato](https://togithub.com/RustyYato) made their first contribution in [https://github.com/maciejhirsz/logos/pull/376](https://togithub.com/maciejhirsz/logos/pull/376) - [@​Jakobeha](https://togithub.com/Jakobeha) made their first contribution in [https://github.com/maciejhirsz/logos/pull/390](https://togithub.com/maciejhirsz/logos/pull/390) - [@​lukas-code](https://togithub.com/lukas-code) made their first contribution in [https://github.com/maciejhirsz/logos/pull/393](https://togithub.com/maciejhirsz/logos/pull/393) - [@​joerivanruth](https://togithub.com/joerivanruth) made their first contribution in [https://github.com/maciejhirsz/logos/pull/396](https://togithub.com/maciejhirsz/logos/pull/396) - [@​afreeland](https://togithub.com/afreeland) made their first contribution in [https://github.com/maciejhirsz/logos/pull/379](https://togithub.com/maciejhirsz/logos/pull/379) - [@​LeoDog896](https://togithub.com/LeoDog896) made their first contribution in [https://github.com/maciejhirsz/logos/pull/403](https://togithub.com/maciejhirsz/logos/pull/403) **Full Changelog**: maciejhirsz/logos@v0.14...v0.14.1 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR was generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View the [repository job log](https://developer.mend.io/github/akrantz01/antsi). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy40NDAuNyIsInVwZGF0ZWRJblZlciI6IjM3LjQ0MC43IiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6W119--> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
](https://renovatebot.com){kind=link}
Fixes #385
This removes unreachable branches from LUTs in the generated code, so that LLVM can see that the error case is unreachable for lexers like in the linked issue